Workflow Registry: Add remaining tests for workflow registry manager #15359
Conversation
|
I see you updated files related to |
eutopian
force-pushed
the
add-more-workflow-registry-tests
branch
from
1df2084 to
a14a469
Compare
eutopian
force-pushed
the
add-more-workflow-registry-tests
branch
12 times, most recently
from
9052490 to
ab849b7
Compare
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
| @@ -83,8 +83,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { | |||
| bytes32 indexed workflowID, address indexed workflowOwner, uint32 indexed donID, string workflowName | |||
| ); | |||
| event WorkflowForceUpdateSecretsRequestedV1(address indexed owner, bytes32 secretsURLHash, string workflowName); | |||
| event RegistryLockedV1(address indexed lockedBy); | |||
There was a problem hiding this comment.
The event is highly infrequent and the index won't help much with anything except additional gas cost
There was a problem hiding this comment.
And this will always be the contract owner only, right?
There was a problem hiding this comment.
Now, when I think about it again, maybe this event doesn't even have to emit this information. The owner will always be an MSIG contract, probably MCMS.
contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.activateWorkflow.t.sol
Outdated
Show resolved
Hide resolved
...cts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.getAllAuthorizedAddresses.t.sol
Show resolved
Hide resolved
contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.lockRegistry.t.sol
Outdated
Show resolved
Hide resolved
contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.unlockRegistry.t.sol
Outdated
Show resolved
Hide resolved
eutopian
force-pushed
the
add-more-workflow-registry-tests
branch
5 times, most recently
from
c1de9aa to
ef5d732
Compare
eutopian
force-pushed
the
add-more-workflow-registry-tests
branch
6 times, most recently
from
802b0ab to
a067c24
Compare
| @@ -2,7 +2,7 @@ WorkflowRegistry.getWorkflowMetadataListByOwner | |||
| ├── when the owner has workflows | |||
| │ ├── when start is 0 | |||
| │ │ └── it returns the correct metadata list | |||
| │ ├── when start is greater than 0 and limit exceeds total | |||
| │ ├── when start is greater than 0 | |||
| │ │ └── it returns the correct metadata list | |||
| │ ├── when limit is less than total workflows | |||
There was a problem hiding this comment.
What about when the limit is set to zero? I assume when you say limit, you mean page size? Perhaps this test is also missing in other view functions that return paged data.
| s_registryManager.addVersion(address(mockWfrContract), s_chainID, s_deployedAt, true); | ||
|
|
||
| // Get the latest version number again, which should be 1. | ||
| uint32 versionNumber = s_registryManager.getLatestVersionNumber(); |
There was a problem hiding this comment.
Nit: maybe there would be more differences between this test and test_WhenAutoActivateIsFalse if another active version already exists on the contract. That means this has to return 2, and for the other test, it has to return 1.
| s_registryManager.getLatestVersionNumber(); | ||
|
|
||
| // Deploy a MockWorkflowRegistryContract contract | ||
| MockWorkflowRegistryContract mockWfrContract = new MockWorkflowRegistryContract(); |
| _deployMockRegistryAndAddVersion(true); | ||
| uint32 versionNumber = | ||
| s_registryManager.getVersionNumberByContractAddressAndChainID(address(s_mockWorkflowRegistryContract), s_chainID); | ||
| assertEq(versionNumber, 1); |
There was a problem hiding this comment.
What happens if I want to register a new version for the same WR contract on the same chain? Does that mean that getVersionNumberByContractAddressAndChainID will return the new version number after I register it (maybe adding a test for that)? And will my previous version forever be lost or can I still see it on the contract, e.g. via getAllVersions?
There was a problem hiding this comment.
Hm but if the contract address is the same and the chain id is the same then it can just activate the old one? I guess we don't have validation on uniqueness I think of (chain id + contract address) but I feel like on the product level that makes sense.
There was a problem hiding this comment.
will punt on making the changes to make it unique but right now since the key is unique there can only be one anyway so it will just override the version with the new one
eutopian
force-pushed
the
add-more-workflow-registry-tests
branch
from
a067c24 to
176a001
Compare
WorkflowRegistryManagerVersionAlreadyActivewhen trying to activate the current active version inWorkflowRegistryManager